Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the clusterupgrade feature config for the fleet Feature resource. #9614

Merged

Conversation

kapreus
Copy link
Contributor

@kapreus kapreus commented Dec 9, 2023

Add the clusterupgrade feature config for the fleet Feature resource. Fixes hashicorp/terraform-provider-google#16459

Release Note Template for Downstream PRs (will be copied)

gkehub2: added `clusterupgrade` to `google_gke_hub_feature` resource.

@modular-magician
Copy link
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will require approval to run.

@shuyama1, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Dec 9, 2023
@kapreus kapreus marked this pull request as ready for review December 9, 2023 06:26
@shuyama1
Copy link
Member

@kapreus Thanks for working on this feature. Would you mind following the Terraform internal contribution page to get your GitHub account registered, so the tests will be automatically triggered for your PRs. Thanks!

@modular-magician modular-magician added service/gkehub and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Dec 11, 2023
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 542 insertions(+))
Terraform Beta: Diff ( 3 files changed, 542 insertions(+))
TF Conversion: Diff ( 1 file changed, 153 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3291
Passed tests 2953
Skipped tests: 338
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Dec 11, 2023
@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Dec 12, 2023
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 542 insertions(+))
Terraform Beta: Diff ( 3 files changed, 542 insertions(+))
TF Conversion: Diff ( 1 file changed, 153 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3291
Passed tests 2953
Skipped tests: 338
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. Only some small comments

Comment on lines 237 to 244
- !ruby/object:Api::Type::String
name: 'name'
description: |
Name of the upgrade, e.g., "k8s_control_plane". It should be a valid upgrade name. It must not exceet 99 characters.
- !ruby/object:Api::Type::String
name: 'version'
description: |
Version of the upgrade, e.g., "1.22.1-gke.100". It should be a valid version. It must not exceet 99 characters.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make at least one of these subfields required to prevent users from sending empty blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, made both of the fields required

location = "global"
spec {
clusterupgrade {
upstream_fleets = [tostring(google_project.project_2.number)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
upstream_fleets = [tostring(google_project.project_2.number)]
upstream_fleets = [google_project.project_2.number]

I'd assume it should be working without tostring since number should be type of string: https://github.com/hashicorp/terraform-provider-google-beta/blob/main/google-beta/services/resourcemanager/resource_google_project.go#L102

Plus, just want to make sure if it will return the values in the order as they send for multiple entries. Otherwise, we'll need to implement it as set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
The order doesn't change; also, right now having more than upstreams is not supported

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 542 insertions(+))
Terraform Beta: Diff ( 3 files changed, 542 insertions(+))
TF Conversion: Diff ( 1 file changed, 153 insertions(+))

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 543 insertions(+))
Terraform Beta: Diff ( 3 files changed, 543 insertions(+))
TF Conversion: Diff ( 1 file changed, 153 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3311
Passed tests 2974
Skipped tests: 337
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM! only one small nit-pick

Comment on lines 538 to 539
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"update_time"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"update_time"},
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"update_time"},

nit: tabs for indentation here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 543 insertions(+))
Terraform Beta: Diff ( 3 files changed, 543 insertions(+))
TF Conversion: Diff ( 1 file changed, 153 insertions(+))

@kapreus kapreus force-pushed the gkehub-clusterupgrade-feature branch from 48aecb1 to e24bbaa Compare January 2, 2024 23:25
@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 23
Passed tests 13
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages
  • gkehub2

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 23
Passed tests 13
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages
  • gkehub2

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@shuyama1 shuyama1 merged commit 1cb7c59 into GoogleCloudPlatform:main Jan 9, 2024
12 checks passed
bskaplan pushed a commit to bskaplan/magic-modules that referenced this pull request Jan 17, 2024
…GoogleCloudPlatform#9614)

* Add support for gkehub clusterupgrade feature

* Fix gkehub Feature indentation

* Remove unnecessary tostring() call

* Make upgrade type and version fields required for specifying overrides

* Ignore update_time in tests

* Replace tabulations with spaces
kylase pushed a commit to yuanchuankee/magic-modules that referenced this pull request Jan 21, 2024
…GoogleCloudPlatform#9614)

* Add support for gkehub clusterupgrade feature

* Fix gkehub Feature indentation

* Remove unnecessary tostring() call

* Make upgrade type and version fields required for specifying overrides

* Ignore update_time in tests

* Replace tabulations with spaces
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
…GoogleCloudPlatform#9614)

* Add support for gkehub clusterupgrade feature

* Fix gkehub Feature indentation

* Remove unnecessary tostring() call

* Make upgrade type and version fields required for specifying overrides

* Ignore update_time in tests

* Replace tabulations with spaces
pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024
…GoogleCloudPlatform#9614)

* Add support for gkehub clusterupgrade feature

* Fix gkehub Feature indentation

* Remove unnecessary tostring() call

* Make upgrade type and version fields required for specifying overrides

* Ignore update_time in tests

* Replace tabulations with spaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for managing GKE Upgrade Rollout Sequencing to google_gke_hub_feature
3 participants